apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (5)

7-7: Remove duplicate Option import

The Option type is imported individually and also as part of the destructured import from '@island.is/island-ui/core'.

-import { Box, Button, Option, Select, Stack } from '@island.is/island-ui/core'
+import { Box, Button, Select, Stack } from '@island.is/island-ui/core'

---

Line range hint 52-56: Clarify the empty onContinue handler

The empty onContinue handler in the Modal component could be confusing. Consider removing it if not needed, as the form submission is handled by the onSubmit prop.

-      onContinue={() => {
-        // Intentionally left empty
-      }}
+      onContinue={undefined}

---

Line range hint 124-131: Simplify email validation pattern

The current email validation regex is complex and might miss some valid email formats. Consider using a simpler pattern or a dedicated email validation library.

               pattern: {
-                value:
-                  /^[\w!#$%&'*+/=?`{|}~^-]+(?:\.[\w!#$%&'*+/=?`{|}~^-]+)*@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$/i,
+                value: /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i,
                 message: t.modal.inputs.email.rules?.validate,
               },

---

Line range hint 196-207: Improve responsive design implementation

The Box component uses hardcoded padding values which might not scale well across different screen sizes. Consider using theme-based spacing values for better maintainability.

-          <Box paddingX={[3, 3, 3, 15]}></Box>
+          <Box paddingX={[2, 3, 4, 5]}></Box>

---

Line range hint 30-209: Add loading state handling

The form submission lacks loading state handling, which could lead to multiple submissions. Consider adding a loading state to disable the form during submission.

You could:

1. Add a loading prop to the component
2. Disable the submit button and form inputs during submission
3. Show a loading indicator

Would you like me to provide a detailed implementation?

apps/skilavottord/ws/seeders/20201010211010-seed-recycling-request.js (1)

Line range hint 1-96: Consider adding documentation for test data scenarios

The seeder file contains various test scenarios (different request_types and recycling_partner_id states) but lacks documentation explaining the purpose of each scenario. Consider adding comments to explain:

1. The significance of null vs non-null recycling_partner_id
2. The progression of request_types
3. The relationship between dates and states

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (3)

55-70: Simplify conditional logic for route variables

The logic setting breadcrumbTitle, title, info, activeSection, and route can be simplified for better readability and maintainability.

Consider refactoring as follows:

 const isMunicipality = router.route === routes.municipalities.add

 const {
   t: { recyclingCompanies: t, municipalities: mt, routes },
 } = useI18n()

 const breadcrumbTitle = isMunicipality ? mt.title : t.title
 const title = isMunicipality ? mt.municipality.add.title : t.recyclingCompany.add.title
 const info = isMunicipality ? mt.municipality.add.info : t.recyclingCompany.add.info
 const activeSection = isMunicipality ? 1 : 2
 const route = isMunicipality ? routes.municipalities.baseRoute : routes.recyclingCompanies.baseRoute

---

117-125: Duplicate code in navigation after creation

The routing logic after a successful creation is duplicated for municipalities and recycling companies.

Refactor to eliminate duplication:

 if (!errors) {
-  if (isMunicipality) {
-    router.push(routes.municipalities.baseRoute).then(() => {
-      toast.success(t.recyclingCompany.add.added)
-    })
-  } else {
-    router.push(routes.recyclingCompanies.baseRoute).then(() => {
-      toast.success(t.recyclingCompany.add.added)
-    })
-  }
+  router.push(route).then(() => {
+    toast.success(t.recyclingCompany.add.added)
+  })
 }

---

129-135: Simplify handleCancel routing logic

The handleCancel function contains duplicated routing logic based on isMunicipality.

Refactor to use the route variable:

 const handleCancel = () => {
-  if (isMunicipality) {
-    router.push(routes.municipalities.baseRoute)
-  } else {
-    router.push(routes.recyclingCompanies.baseRoute)
-  }
+  router.push(route)
 }

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (2)

59-72: Simplify context-based variables

The variables isMunicipality, buttonText, activeSection, title, and info can be consolidated for clearer logic.

Consider refactoring:

 const isMunicipality = router.route === routes.municipalities.baseRoute

 const buttonText = isMunicipality ? t.buttons.addMunicipality : t.buttons.add
 const activeSection = isMunicipality ? 1 : 2
 const title = isMunicipality ? mt.title : t.title
 const info = isMunicipality ? '' : t.info

---

101-109: Duplicate code in handleCreate function

The handleCreate function contains duplicated routing logic.

Refactor to use the routes object based on isMunicipality:

 const handleCreate = () => {
   router.push({
-    pathname: isMunicipality ? routes.municipalities.add : routes.recyclingCompanies.add,
+    pathname: route.add,
   })
 }

Ensure you define route similarly to how it's done in other components.

apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (3)

53-57: Avoid unnecessary default assignment

The default assignment || [] is unnecessary when using skip in the query.

Remove the redundant assignment.

 const { data } =
-  useQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, {
+  useQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, {
     skip: isMunicipality,
     variables: { isMunicipality: true },
   })
-  || []

---

278-311: Conditional rendering might cause layout shift

Conditional rendering of the Select component may cause layout shifts. Consider rendering the component but disabling it or hiding it with CSS to maintain consistent layout.

Alternatively, adjust the layout to accommodate the dynamic content.

---

374-375: Button icon mismatch

The Button component's icon has been changed to save. Ensure this aligns with the action, and consider using the arrowForward icon for consistency unless saving is appropriate here.

apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)

125-147: Unused imports and variables

There are unused imports and variables, such as municipalities, use from 'dd-trace', and potentially others.

Remove unused imports and variables to clean up the code.

apps/skilavottord/web/pages/en/municipalities/index.tsx (1)

1-5: Consider improving type safety and import paths

While the implementation is functional, consider these improvements:

1. Define the Screen type explicitly on the RecyclingCompanies component instead of using type casting
2. Consider using path aliases for imports to improve maintainability

Example implementation:

- import { Screen } from '@island.is/skilavottord-web/types'
+ import { Screen } from '@types'
- import { withLocale } from '@island.is/skilavottord-web/i18n'
+ import { withLocale } from '@i18n'
- import { RecyclingCompanies } from '@island.is/skilavottord-web/screens'
+ import { RecyclingCompanies } from '@screens'

- export default withLocale('en')(RecyclingCompanies as Screen)
+ export default withLocale('en')<Screen>(RecyclingCompanies)

apps/skilavottord/web/pages/en/municipalities/add.tsx (1)

1-5: Apply consistent type safety improvements

Similar to the index page, consider improving type safety and import paths:

Example implementation:

- import { Screen } from '@island.is/skilavottord-web/types'
+ import { Screen } from '@types'
- import { withLocale } from '@island.is/skilavottord-web/i18n'
+ import { withLocale } from '@i18n'
- import { RecyclingCompanyCreate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyCreate'
+ import { RecyclingCompanyCreate } from '@screens/RecyclingCompanies/Create'

- export default withLocale('en')(RecyclingCompanyCreate as Screen)
+ export default withLocale('en')<Screen>(RecyclingCompanyCreate)

apps/skilavottord/web/pages/en/municipalities/[id].tsx (1)

1-5: Consider creating a shared page component

The code is identical to the Icelandic version except for the locale parameter. Consider creating a shared page component to reduce duplication:

+ // shared/MunicipalityPage.tsx
+ import { Screen } from '@island.is/skilavottord-web/types'
+ import { withLocale } from '@island.is/skilavottord-web/i18n'
+ import { RecyclingCompanyUpdate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyUpdate'
+
+ export const createMunicipalityPage = (locale: string) =>
+   withLocale(locale)(RecyclingCompanyUpdate as Screen)

Then both locale pages could simply import and use this shared component:

import { createMunicipalityPage } from '../shared/MunicipalityPage'
export default createMunicipalityPage('en') // or 'is'

apps/skilavottord/ws/src/app/modules/auth/user.model.ts (1)

Line range hint 1-28: Consider adding role documentation

The Role enum would benefit from JSDoc comments describing the permissions and responsibilities of each role, especially the newly added municipality role.

 export enum Role {
+  /** System administrator with full access */
   developer = 'developer',
+  /** Company that processes recycled vehicles */
   recyclingCompany = 'recyclingCompany',
+  /** Administrative user for recycling companies */
   recyclingCompanyAdmin = 'recyclingCompanyAdmin',
+  /** Recycling fund administrator */
   recyclingFund = 'recyclingFund',
+  /** Regular user accessing the system */
   citizen = 'citizen',
+  /** Municipality administrator with specific access rights */
   municipality = 'municipality',
 }

apps/skilavottord/web/components/PageHeader/PageHeader.tsx (2)

11-14: Consider enhancing props interface documentation

The props interface could be more descriptive to improve maintainability.

-export const PageHeader: FC<{ title: string; info: string }> = ({
+interface PageHeaderProps {
+  /** Main heading text for the page */
+  title: string;
+  /** Additional information to display in tooltip */
+  info: string;
+}
+
+export const PageHeader: FC<PageHeaderProps> = ({
   title,
   info,
 }) => {

---

17-26: Consider layout shift optimization

The current order switching between mobile and desktop might cause layout shift during page load.

Consider using CSS Grid's grid-template-areas instead of order properties for a more stable layout that doesn't cause Content Layout Shift (CLS).

apps/skilavottord/web/utils/roleUtils.ts (2)

18-19: Enhance default case handling

The default case using startCase might not be ideal for all future role values.

-    default:
-      return startCase(role)
+    default: {
+      console.warn(`Translation missing for role: ${role}`);
+      return startCase(role);
+    }

---

Missing test coverage for role translations

The codebase lacks dedicated test coverage for the getRoleTranslation utility function. While there is a test file in the web directory (locales.spec.ts), it doesn't cover the role translations. The function handles critical UI text for different user roles, including the newly added municipality role, and should be properly tested for both Icelandic and English translations.

Suggested actions:

• Create a new test file roleUtils.spec.ts in apps/skilavottord/web/utils/
• Add test cases covering all role translations in both languages
• Include specific test cases for the new municipality role translations ('Sveitarfélag'/'Municipality')

🔗 Analysis chain

Line range hint 6-20: Verify test coverage for new role translation

The new municipality role translation needs test coverage to ensure correct behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing test files and their coverage
fd -e test.ts -e spec.ts roleUtils apps/skilavottord/web/utils/

Length of output: 65

---

Script:

#!/bin/bash
# Let's try a broader search for test files in the web directory
fd -e test.ts -e spec.ts apps/skilavottord/web

# Also search for any usage of getRoleTranslation in test files
rg "getRoleTranslation" -t ts --glob "*test*" --glob "*spec*"

# Search for any test files containing "role" or "Role" to ensure we don't miss any
rg "role|Role" -t ts --glob "*test*" --glob "*spec*" apps/skilavottord/web

Length of output: 577

---

Script:

#!/bin/bash
# Search for test files in the correct way
fd . apps/skilavottord/web -e test.ts -e spec.ts

# Look for any test files containing Role enum
ast-grep --pattern 'enum Role {
  $$$
}'

# Search for any files importing roleUtils to see test coverage
rg -l "from ['|\"].*roleUtils" 

Length of output: 1261

apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (2)

29-30: Remove unnecessary template literals

Template literals are used unnecessarily for static string values. Since these strings don't contain any interpolation, they can be simplified.

-          title: `${sidenavText.recycled}`,
-          link: `${routes.recycledVehicles}`,
+          title: sidenavText.recycled,
+          link: routes.recycledVehicles,

Apply similar changes to other occurrences.

Also applies to: 34-35, 40-41, 45-46

---

7-11: Define interface for component props

Consider extracting the props interface for better type documentation and reusability.

+interface NavigationLinksProps {
+  activeSection: number;
+}
+
-export const NavigationLinks = ({
-  activeSection,
-}: {
-  activeSection: number
-}) => {
+export const NavigationLinks = ({ activeSection }: NavigationLinksProps) => {

apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts (2)

108-108: Standardize comment style

The inline comment style differs from the rest of the file. Consider removing it since the @Field decorator with nullable: true is self-documenting.

-  @Field({ nullable: true }) // Ensure this field is nullable in GraphQL
+  @Field({ nullable: true })

---

101-115: Consider grouping related fields

The new municipality-related fields could be grouped with other identifying fields at the top of the model for better organization.

Consider moving these fields after the companyId field to keep related identifying fields together.

apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)

32-32: Remove redundant comment

The comment restates what's evident from the code and parameter names.

-    // If Role.municipality, return all recycling partners that are not municipalities

apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts (1)

21-32: Consider improving role-based filtering logic

While the role-based filtering is functionally correct, the logic could be more explicit and maintainable.

Consider refactoring to:

-  partnerId: user.role === Role.municipality ? user.partnerId : null,
+  partnerId: user.role === Role.municipality 
+    ? user.partnerId 
+    : (user.role === Role.recyclingFund ? null : user.partnerId),

This makes the intent clearer for each role and improves maintainability.

apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)

Line range hint 43-65: Consider implementing pagination optimization

The current implementation might face performance issues with large datasets, especially when filtering across multiple partners.

Consider these optimizations:

1. Add database indexes for frequently filtered columns
2. Implement cursor-based pagination instead of offset-based
3. Consider caching frequently accessed partner relationships

apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (1)

20-25: Consider organizing roles by privilege level

The roles array would be more maintainable if organized by privilege level.

Consider reorganizing roles:

   roles: [
     Role.developer,
     Role.recyclingFund,
+    Role.municipality,
     Role.recyclingCompanyAdmin,
-    Role.municipality,
   ],

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)

149-151: Add type validation for municipalityId.

The current type checking could be more robust.

-    if (typeof input.municipalityId !== 'string') {
-      input.municipalityId = input.municipalityId?.value || ''
-    }
+    input.municipalityId = typeof input.municipalityId === 'string' 
+      ? input.municipalityId 
+      : input.municipalityId?.value || ''

apps/skilavottord/web/i18n/locales/is.json (2)

368-372: Ensure consistent navigation structure.

The new municipality-related entries in the sidenav section should maintain hierarchy consistency with other sections.

Consider this structure for better consistency:

  "recyclingFundSidenav": {
    "title": "Úrvinnslusjóður",
-   "municipalityTitle": "Sveitarfélag",
    "recycled": "Meðhöndluð ökutæki",
    "companies": "Móttökuaðilar",
    "accessControl": "Aðgangsstýring",
    "municipalities": "Sveitarfélög"
+   "municipality": {
+     "title": "Sveitarfélag"
+   }
  }

---

415-425: Improve form field labels and placeholders.

The form input labels and placeholders for company ID and name could be more descriptive.

Consider these improvements:

  "companyId": {
-   "label": "Auðkenni (ID)",
-   "placeholder": "Auðkenni",
+   "label": "Auðkenni fyrirtækis (ID)",
+   "placeholder": "Sláðu inn einkvæmt auðkenni",
    "rules": {
      "required": "Skylda er að fylla út auðkenni"
    }
  },
  "companyName": {
-   "label": "Nafn",
-   "placeholder": "Nafn",
+   "label": "Nafn fyrirtækis",
+   "placeholder": "Sláðu inn nafn fyrirtækis",
    "rules": {
      "required": "Skylda er að fylla út nafn"
    }
  }

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for vehicle ID validation patterns in the codebase
rg -g '!*.{json,md,lock}' -i 'vehicle.*validation|vehicle.*pattern|vehicle.*format' 

# Look for any existing vehicle ID usage to confirm format consistency
rg -g '!*/seeders/*' 'vehicle_id.*[A-Z0-9]{2,}' 

#!/bin/bash
# Check if multiple requests for the same vehicle are handled in the application
ast-grep --pattern 'function $FUNC($PARAMS) {
  $$$
  vehicle_id
  $$$
}'

#!/bin/bash
# Search for files that handle recycling requests and vehicle IDs
rg -l "vehicle_id" --type js

# Check the content of relevant files for vehicle ID handling
rg "vehicle_id" -A 3 -B 3 --type js

# Look for any unique constraints or validations on vehicle_id
rg "unique.*vehicle_id|vehicle_id.*unique" --type js

 import { Screen } from '@island.is/skilavottord-web/types'
 import { withLocale } from '@island.is/skilavottord-web/i18n'
-import { RecyclingCompanies } from '@island.is/skilavottord-web/screens'
+import { Municipalities } from '@island.is/skilavottord-web/screens'
+
+interface MunicipalitiesPageProps {
+  // Add required props here
+}
+
+export const getServerSideProps = async () => {
+  // Fetch required data
+  return {
+    props: {
+      // Return fetched data
+    },
+  }
+}
 
-export default withLocale('is')(RecyclingCompanies as Screen)
+const MunicipalitiesPage: Screen<MunicipalitiesPageProps> = (props) => {
+  return <Municipalities {...props} />
+}
+
+export default withLocale('is')(MunicipalitiesPage)

#!/bin/bash
# Description: Check if the server-side GraphQL schema includes 'isMunicipality' and 'municipalityId' in the 'createSkilavottordRecyclingPartner' mutation.

# Fetch the GraphQL schema and search for the fields in the mutation input type.
curl -s http://localhost:3333/api/graphql -H "Content-Type: application/json" \
  -d '{"query": "query { __type(name: \"CreateRecyclingPartnerInput\") { inputFields { name } } }"}' \
  | jq '.data.__type.inputFields[].name' \
  | grep -E 'isMunicipality|municipalityId'

#!/bin/bash
# Let's search for GraphQL schema files and mutations related to recycling partners
fd -e graphql -e gql | xargs cat

# Also search for TypeScript files that might contain the schema definition
rg -l "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput" --type=typescript

# Search for resolver implementations
ast-grep --pattern 'class $_ {
  $$$
  createSkilavottordRecyclingPartner($_) {
    $$$
  }
  $$$
}'

#!/bin/bash
# Let's try a different approach to find the schema and resolver files
fd -e ts | xargs rg -l "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput|isMunicipality|municipalityId"

# Also search for any GraphQL schema files
fd -e graphql | xargs rg "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput|isMunicipality|municipalityId"

#!/bin/bash
# Let's examine the relevant files in the skilavottord app
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts

 const { data, error, loading } = useQuery<Query>(
   SkilavottordAllRecyclingPartnersByTypeQuery,
   {
     variables: {
       isMunicipality,
       municipalityId: partnerId,
     },
+    skip: !user,
   },
 )

- import { Screen } from '@island.is/skilavottord-web/types'
+ import { Screen } from '@types'
- import { withLocale } from '@island.is/skilavottord-web/i18n'
+ import { withLocale } from '@i18n'
- import { RecyclingCompanyCreate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyCreate'
+ import { RecyclingCompanyCreate } from '@screens/RecyclingCompanies/Create'

- export default withLocale('is')(RecyclingCompanyCreate as Screen)
+ export default withLocale('is')<Screen>(RecyclingCompanyCreate)

#!/bin/bash
# Check if language switching is properly configured in next.config.js
echo "Checking Next.js i18n configuration..."
cat next.config.js | grep -A 10 "i18n:"

# Verify that both language versions are properly linked
echo "Checking for language switcher component usage..."
rg -l "LanguageSwitcher" apps/skilavottord/web/

#!/bin/bash
# Let's try to find the Next.js config file with different possible names and locations
echo "Searching for Next.js config files..."
fd -t f "next.config" apps/skilavottord/web/

# Check for i18n configuration in package.json as an alternative
echo "Checking package.json for i18n config..."
fd -t f "package.json" apps/skilavottord/web/ -x cat {} | jq -r '.i18n'

# Let's check for language-related components and configurations
echo "Checking for language-related files..."
rg -l "i18n|locale|language" apps/skilavottord/web/

# Check for the English version of the same page
echo "Checking for English version of municipalities/add..."
fd -t f "add.tsx" apps/skilavottord/web/pages/en/

#!/bin/bash
# Let's check the Next.js config content for i18n settings
echo "Checking Next.js config content..."
cat apps/skilavottord/web/next.config.js

# Let's examine the i18n implementation
echo "Checking i18n implementation..."
cat apps/skilavottord/web/i18n/withLocale.tsx

# Compare both language versions of the add page
echo "Comparing both language versions..."
echo "=== Icelandic version ==="
cat apps/skilavottord/web/pages/municipalities/add.tsx
echo "=== English version ==="
cat apps/skilavottord/web/pages/en/municipalities/add.tsx

#!/bin/bash
# Check if RecyclingCompanyUpdate component handles municipality role
ast-grep --pattern 'interface $_ {
  $$$
  municipality?: $_
  $$$
}'

#!/bin/bash
# Check RecyclingCompanyUpdate component implementation
ast-grep --pattern 'export const RecyclingCompanyUpdate = $_'

# Search for municipality-related props or types in the component
rg -A 5 "RecyclingCompanyUpdate" apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate.tsx

# Check if there are other municipality-specific components
fd "Municipality" apps/skilavottord/web

#!/bin/bash
# Find all files containing RecyclingCompanyUpdate
rg -l "RecyclingCompanyUpdate"

# Find municipality-related components and pages
fd -e tsx -e ts "municipality" -i

# Check imports in the municipality page
cat apps/skilavottord/web/pages/municipalities/[id].tsx

#!/bin/bash
# Check the implementation of RecyclingCompanyUpdate component
cat apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx

# Check if there are separate components for municipalities
cat apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/index.tsx

#!/bin/bash
# Check for proper RBAC implementation for municipality role
echo "Checking resolvers for municipality role authorization..."
rg -l "Role.municipality" --type ts

echo "\nChecking guards and decorators..."
ast-grep --pattern '@$_($$$)
export class $_ {
  $$$
  @$_([Role.municipality])
  $$$
}'

#!/bin/bash
# Check resolver implementations for municipality role
echo "Checking vehicle resolver implementation..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavortord/ws/src/app/modules/vehicle/vehicle.resolver.ts

echo "\nChecking access control resolver implementation..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavortord/ws/src/app/modules/accessControl/accessControl.resolver.ts

echo "\nChecking for role-based guards..."
rg -l "RolesGuard" --type ts

echo "\nChecking guard implementations..."
ast-grep --pattern 'class $_ implements CanActivate {
  $$$
  canActivate($$$) {
    $$$
  }
}'

#!/bin/bash
# Check roles guard implementation in skilavottord
echo "Checking roles guard implementation..."
rg -B 2 -A 10 "class RolesGuard" apps/skilavottord/ws/src/app/modules/auth/roles.guard.ts

echo "\nChecking for roles decorator usage..."
rg -B 2 -A 5 "@Roles" apps/skilavottord/ws/src/app/modules/

echo "\nChecking recycling partner resolver for role checks..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts

ALTER TABLE recycling_partner 
ADD CONSTRAINT fk_recycling_partner_municipality 
FOREIGN KEY (municipality_id) 
REFERENCES municipality(id);

#!/bin/bash
# Search for UI components that might need updates for the new 'municipalities' permission
rg -t tsx -t ts "municipalities" apps/skilavottord/web/

#!/bin/bash
# Search for UI components that might need updates for the new 'municipalities' permission
# Using ripgrep without file type restrictions and checking the entire web directory
rg "municipalities" apps/skilavottord/web/

# Also search for any components or code handling permissions
rg -A 3 "hasPermission|usePermissions" apps/skilavottord/web/

# Check for any routes or components related to municipalities
rg -i "municipality|municipalities" apps/skilavottord/web/

#!/bin/bash
# Description: Check for potential null handling issues in the codebase

# Check for explicit null checks in similar service methods
rg -A 2 "findAll.*null" apps/skilavottord/ws/src/app/modules

# Check for related model validations
ast-grep --pattern 'allowNull: true' apps/skilavottord/ws/src/app/modules

#!/bin/bash
# Let's check the model definition and related files for null handling

# Find the recycling partner model definition
fd "recyclingPartner.*model" apps/skilavottord/ws/src

# Check for null handling in the model and related files
rg -g "*.ts" "isMunicipality.*null|municipalityId.*null" apps/skilavottord/ws/src/app/modules

# Check for any validation or type definitions
rg -g "*.ts" "interface.*RecyclingPartner|type.*RecyclingPartner" apps/skilavottord/ws/src

#!/bin/bash
# Let's examine the model definition and input types

# Check the model definition
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts

# Check the input types
rg -g "*.ts" -A 5 "RecyclingPartnerInput|CreateRecyclingPartnerInput|UpdateRecyclingPartnerInput" apps/skilavottord/ws/src/app/modules/recyclingPartner/

#!/bin/bash
# Search for role checks in the codebase
rg "Role\.municipality" --type ts

# Look for access control implementations
ast-grep --pattern 'class $_ implements CanActivate {
  $$$
  canActivate($_) {
    $$$
  }
}'

#!/bin/bash
# Check how municipality access is restricted in services
rg "Role\.municipality.*partnerId" -A 5 --type ts

# Look for partnerId checks in resolvers
ast-grep --pattern 'if ($user.role === Role.municipality) {
  $$$
}'

# Check authorization decorator implementation
rg "@Authorize" -A 3 --type ts

#!/bin/bash
# Check recycling partner service implementation
rg -A 10 "class RecyclingPartnerService" --type ts

# Look for municipality-specific filtering in the service
rg -A 10 "Role\.municipality.*return" --type ts

# Check how partnerId is used in queries
ast-grep --pattern 'findOne({
  $$$
  partnerId: $user.partnerId
  $$$
})'

#!/bin/bash
# Check for potential security implications of municipality role
# Look for other endpoints that might need municipality role

# Search for @Authorize decorators
rg -B 2 -A 2 "@Authorize\({" 

# Search for role-based checks
rg "user\.role === " 

#!/bin/bash
# Search for Authorize decorators with better regex
rg "@Authorize\(" -A 2

# Search for municipality role usage
rg "Role\.municipality"

# Search for role-based mutations/queries in vehicle resolver
ast-grep --pattern $'@Authorize({ roles: [$$$] })
@$_($$$) $$$'

-  React.useEffect(() => {
-    setValue('isMunicipality', isMunicipality)
-  }, [isMunicipality, setValue])
+  React.useEffect(() => {
+    setValue('isMunicipality', isMunicipality)
+  }, [isMunicipality, setValue])

#!/bin/bash
# Check for proper role definitions and access control
rg -A 5 "Role.municipality"

#!/bin/bash
# Check for authorization decorators and role checks in recycling partner related files
ast-grep --pattern 'Authorize({ roles: [$$$] })'

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (3)

84-90: Consider a more robust route check

The current route check for isMunicipality is tightly coupled to the exact route path. Consider using a more maintainable approach.

Consider this alternative:

- const isMunicipality = router.route === routes.municipalities.edit
+ const isMunicipality = router.route.startsWith(routes.municipalities.baseRoute)

---

119-124: Move string literals to translation constants

Consider moving the hardcoded section number and route to translation constants for better maintainability.

Example approach:

const SECTIONS = {
  RECYCLING_COMPANIES: 2,
  MUNICIPALITIES: 1,
} as const

---

Line range hint 156-164: Enhance error handling in mutation response

The current error handling doesn't provide user feedback when the mutation fails.

Add error handling:

  const { errors } = await updateSkilavottordRecyclingPartner({
    variables: { input },
  })
  if (!errors) {
    router.push(routes.recyclingCompanies.baseRoute).then(() => {
      toast.success(t.recyclingCompany.view.updated)
    })
+ } else {
+   toast.error(t.recyclingCompany.view.updateError)
  }

🧰 Tools

🪛 Biome (1.9.4)

[error] 150-150: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (2)

12-21: Consider adding null safety to role check

The role check could be made more robust by adding null safety:

-  if (hasMunicipalityRole(user?.role)) {
+  if (user?.role && hasMunicipalityRole(user.role)) {

---

23-52: Remove unnecessary template literals

Template literals are used unnecessarily for static strings in the sections array.

-          title: `${sidenavText.recycled}`,
-          link: `${routes.recycledVehicles}`,
+          title: sidenavText.recycled,
+          link: routes.recycledVehicles,

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)

58-73: Consider extracting route comparison logic

The route comparison could be made more maintainable by using a constant.

+const MUNICIPALITY_ROUTE = routes.municipalities.add;
+
-const isMunicipality = router.route === routes.municipalities.add
+const isMunicipality = router.route === MUNICIPALITY_ROUTE

apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (2)

123-145: Consolidate data fetching logic for recycling partners

The current implementation uses two separate useLazyQuery hooks (getAllRecyclingPartner and getAllRecyclingPartnersByMunicipality) to fetch recycling partner data based on the user's role. This can be simplified by combining the logic into a single hook or function that handles the condition internally. This will reduce code duplication and improve maintainability.

Consider refactoring as follows:

- const [
-   getAllRecyclingPartner,
-   {
-     data: recyclingPartnerData,
-     error: recyclingPartnerError,
-     loading: recyclingPartnerLoading,
-   },
- ] = useLazyQuery<Query>(SkilavottordAllRecyclingPartnersQuery, {
-   ssr: false,
- })
- 
- const [
-   getAllRecyclingPartnersByMunicipality,
-   {
-     data: recyclingPartnerByIdData,
-     error: recyclingPartnerByIdError,
-     loading: recyclingPartnerByIdLoading,
-   },
- ] = useLazyQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, {
-   ssr: false,
-   variables: { isMunicipality: false, municipalityId: user?.partnerId },
- })
+ const [
+   getRecyclingPartners,
+   {
+     data: recyclingPartnerData,
+     error: recyclingPartnerError,
+     loading: recyclingPartnerLoading,
+   },
+ ] = useLazyQuery<Query>(
+   user.role === Role.municipality
+     ? SkilavottordAllRecyclingPartnersByTypeQuery
+     : SkilavottordAllRecyclingPartnersQuery,
+   {
+     ssr: false,
+     variables:
+       user.role === Role.municipality
+         ? { isMunicipality: false, municipalityId: user?.partnerId }
+         : {},
+   },
+ )

---

226-228: Handle undefined municipalityId safely in labels

In constructing the label for recyclingPartners, the ternary check on partner.municipalityId should ensure that undefined or null values are handled properly to avoid displaying undefined - companyName.

Consider using a more robust nullish coalescing operator:

- label: partner.municipalityId
+ label: partner.municipalityId ?? ''

Or adjust the ternary condition:

- label: partner.municipalityId
+ label: partner.municipalityId

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.

-  if (hasMunicipalityRole(user?.role)) {
+  if (user?.role && hasMunicipalityRole(user.role)) {

+  const getRoutePath = (type: 'create' | 'edit', id?: string) => {
+    const base = isMunicipality ? routes.municipalities : routes.recyclingCompanies;
+    const path = type === 'create' ? base.add : base.edit;
+    return id ? { pathname: path, query: { id } } : { pathname: path };
+  }

   const handleCreate = () => {
-    if (isMunicipality) {
-      router.push({
-        pathname: routes.municipalities.add,
-      })
-    } else {
-      router.push({
-        pathname: routes.recyclingCompanies.add,
-      })
-    }
+    router.push(getRoutePath('create'));
   }

   const handleUpdate = (id: string) => {
-    if (isMunicipality) {
-      router.push({
-        pathname: routes.municipalities.edit,
-        query: { id },
-      })
-    } else {
-      router.push({
-        pathname: routes.recyclingCompanies.edit,
-        query: { id },
-      })
-    }
+    router.push(getRoutePath('edit', id));
   }

const roles = Object.keys(AccessControlRole)
  .filter((role) => {
    if (!hasDeveloperRole(user?.role) && role === Role.developer) {
      return false
    }
    if (user.role === Role.municipality) {
      return [Role.recyclingCompany, Role.recyclingCompanyAdmin, Role.municipality].includes(role)
    }
    return true
  })
  .map((role) => ({
    label: getRoleTranslation(role as Role, activeLocale),
    value: role,
  }))

- const getAllCompanyIds = async (partnerId: string) => {
-   partnerIds.push(partnerId)
-   const subCompanies = await RecyclingPartnerModel.findAll({
-     where: { municipalityId: partnerId },
-   })
-   for (const subCompany of subCompanies) {
-     await getAllCompanyIds(subCompany.companyId)
-   }
- }
+ const getAllCompanyIds = async (
+   partnerId: string,
+   visited = new Set<string>(),
+   depth = 0,
+ ) => {
+   const MAX_DEPTH = 100
+   if (depth > MAX_DEPTH || visited.has(partnerId)) return
+   visited.add(partnerId)
+   partnerIds.push(partnerId)
+   const subCompanies = await RecyclingPartnerModel.findAll({
+     where: { municipalityId: partnerId },
+   })
+   for (const subCompany of subCompanies) {
+     await getAllCompanyIds(subCompany.companyId, visited, depth + 1)
+   }
+ }

apps/skilavottord/ws/src/app/modules/accessControl/accessControl.model.ts (1)

Line range hint 66-73: LGTM! The new partnerId field is well-structured.

The implementation follows TypeScript, GraphQL, and Sequelize best practices with proper decorators and type safety.

Consider adding a JSDoc comment to document the purpose of this field and its relationship with RecyclingPartnerModel:

+ /**
+  * ID of the associated recycling partner. Used to establish relationship
+  * between access control entries and recycling partners for municipality management.
+  */
  @Field({ nullable: true })
  @ForeignKey(() => RecyclingPartnerModel)
  @Column({
    type: DataType.STRING,
    field: 'partner_id',
    allowNull: true,
  })
  partnerId?: string

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)

112-116: Add TypeScript types for query variables

Consider adding explicit TypeScript types for the query variables to improve type safety and maintainability.

+ interface RecyclingPartnersQueryVars {
+   isMunicipalityPage: boolean
+   municipalityId: string | null
+ }

  refetchQueries: [
    {
      query: SkilavottordRecyclingPartnerQuery,
      variables: { input: { companyId: id } },
    },
    {
      query: SkilavottordRecyclingPartnersQuery,
-     variables: {
-       isMunicipalityPage: isMunicipalityPage,
-       municipalityId: partnerId,
-     },
+     variables: {
+       isMunicipalityPage,
+       municipalityId: partnerId,
+     } as RecyclingPartnersQueryVars,
    },
  ],

---

128-130: Add cleanup function to useEffect

Consider adding a cleanup function to handle component unmounting, following React best practices.

  React.useEffect(() => {
    setValue('isMunicipality', isMunicipalityPage)
+   return () => {
+     // Reset the form value on unmount if needed
+     setValue('isMunicipality', false)
+   }
  }, [isMunicipalityPage, setValue])

apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx (1)

Line range hint 52-195: Excellent implementation following NextJS and TypeScript best practices.

The component demonstrates:

• Proper error handling and loading states
• Type-safe implementation
• Efficient data fetching with pagination
• Clean and maintainable structure

Consider implementing React Query or SWR for better cache management and automatic revalidation of data in future iterations.

apps/skilavottord/web/i18n/locales/translation.d.ts (1)

478-493: Consider enhancing type safety for municipality interfaces.

The new Municipalities and Municipality interfaces reuse existing patterns, which is good. However, consider adding more specific types for municipality-specific fields that might have unique validation requirements.

Example enhancement:

 export interface Municipality {
   view: View
   add: Add
-  form: RecyclingCompanyForm
+  form: {
+    inputs: MunicipalityFormInputs
+    buttons: FormButtons
+  }
 }

+export interface MunicipalityFormInputs extends FormInputs {
+  // Add municipality-specific fields with appropriate validation rules
+  region?: {
+    label: string
+    placeholder: string
+    rules: {
+      required: string
+    }
+  }
+}

apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (2)

68-71: Initialize component states in useEffect

The useEffect hook initializes the visibility of company and municipality selection inputs. Confirm that this aligns with the intended initial state when the modal is opened.

Consider whether the useEffect is necessary here, as the initial state values are already set in the useState declarations.

---

73-91: Simplify handleRoleOnChange logic

The handleRoleOnChange function has nested conditions that could be refactored for clarity and maintainability. Consider simplifying the logic to reduce complexity.

Here's a possible refactor:

 const handleRoleOnChange = (e: Option) => {
-  if (hasMunicipalityRole(user?.role)) {
-    if (e && e.value === Role.municipality) {
-      setShowCompaniesSelection(false)
-    } else {
-      setShowCompaniesSelection(true)
-    }
-    setShowMunicipalitiesSelection(false)
-  } else if (hasRecyclingFundRole(user?.role)) {
-    if (e && e.value === Role.municipality) {
-      setShowCompaniesSelection(false)
-      setShowMunicipalitiesSelection(true)
-    } else {
-      setShowMunicipalitiesSelection(false)
-      setShowCompaniesSelection(true)
-    }
-  }
+  const isMunicipalityRoleSelected = e && e.value === Role.municipality
+  if (hasMunicipalityRole(user?.role)) {
+    setShowCompaniesSelection(!isMunicipalityRoleSelected)
+    setShowMunicipalitiesSelection(false)
+  } else if (hasRecyclingFundRole(user?.role)) {
+    setShowCompaniesSelection(!isMunicipalityRoleSelected)
+    setShowMunicipalitiesSelection(isMunicipalityRoleSelected)
+  }
 }

apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (2)

75-78: Consider extracting role-based logic

The inline role check creates tight coupling. Consider extracting this logic to a utility function for better maintainability.

- partnerId:
-   role.value === Role.municipality
-     ? municipalityId?.value
-     : partnerId?.value || null,
+ partnerId: getPartnerId(role.value, municipalityId?.value, partnerId?.value),

---

94-97: Simplify partner ID requirement check

The current check could be simplified using array inclusion.

- partnerIdRequired={
-   watch('role')?.value === Role.recyclingCompanyAdmin ||
-   watch('role')?.value === Role.recyclingCompany
- }
+ partnerIdRequired={[Role.recyclingCompanyAdmin, Role.recyclingCompany].includes(watch('role')?.value)}

apps/skilavottord/web/screens/CompanyInfo/CompanyInfo.tsx (1)

28-28: Consider prop type validation

The PageHeader component is used without TypeScript prop validation. Ensure proper type checking.

Add prop types validation:

interface PageHeaderProps {
  title: string;
  info?: string;
}

Also applies to: 105-105

apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)

278-311: Consider enhancing error handling for municipality selection.

While the implementation is clean, consider adding more specific error messages for municipality selection failures.

 <Select
   name={name}
   label={t.recyclingCompany.form.inputs.municipality.label}
   placeholder={t.recyclingCompany.form.inputs.municipality.placeholder}
   size="md"
   value={municipalities?.find((option) => option.value === value)}
   hasError={!!errors?.municipality?.message}
-  errorMessage={errors?.municipality?.message}
+  errorMessage={errors?.municipality?.message || t.recyclingCompany.form.inputs.municipality.error}
   backgroundColor="blue"
   options={municipalities}
   onChange={onChange}
   isCreatable
 />

apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)

46-48: Consider enhancing error handling for disposalStationId retrieval.

While the addition of disposalStationId retrieval is good, consider adding specific error handling for cases where the ID cannot be retrieved.

 const disposalStationId =
-  await this.recyclingPartnerService.getGetPayingPartnerId(disposalStation)
+  await this.recyclingPartnerService.getGetPayingPartnerId(disposalStation)
+    .catch((error) => {
+      this.logger.error('Failed to retrieve disposal station ID', { error, disposalStation })
+      throw new Error(`Invalid disposal station: ${disposalStation}`)
+    })

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.

#!/bin/bash
# Description: Verify PageHeader component implementation and usage

# Check PageHeader component implementation
ast-grep --pattern 'export const PageHeader = ({ title, info }: Props) => {
  $$$
}'

# Check for consistent usage across the application
rg -A 2 '<PageHeader.*title=.*info=.*>'

#!/bin/bash
# Search for PageHeader component definition
fd PageHeader.tsx --exec cat {}

#!/bin/bash
# Description: Verify route configuration matches implementation
# Test: Search for municipality route definitions in the codebase
rg -t typescript -t javascript "municipalities.*route" ./apps/skilavottord/web

#!/bin/bash
# Search for route configurations and usages of municipalities
rg -g '*.{ts,tsx,js,jsx}' 'municipalities' ./apps/skilavottord/web

#!/bin/bash
# Look for route definitions in common routing files
fd -e ts -e tsx -e js -e jsx "routes|router" ./apps/skilavottord/web --exec cat {}

#!/bin/bash
# Check the type definition and its usage
ast-grep --pattern 'interface $_ {
  $$$
  municipalities: $_
  $$$
}'

 async findRecyclingPartners(
   isMunicipalityPage: boolean,
   municipalityId: string | null,
 ): Promise<RecyclingPartnerModel[]> {
-  // If Role.municipality, return all recycling partners that are not municipalities
-  if (municipalityId) {
-    return await this.recyclingPartnerModel.findAll({
-      where: {
-        isMunicipality: false,
-        municipalityId: municipalityId,
-      },
-    })
-  }
-
-  if (isMunicipalityPage) {
-    return await this.recyclingPartnerModel.findAll({
-      where: { isMunicipality: true },
-    })
-  }
-  return await this.recyclingPartnerModel.findAll({
-    where: {
-      [Op.or]: [{ isMunicipality: false }, { isMunicipality: null }],
-    },
-  })
+  try {
+    const whereCondition = municipalityId
+      ? { isMunicipality: false, municipalityId }
+      : isMunicipalityPage
+      ? { isMunicipality: true }
+      : {
+          [Op.or]: [{ isMunicipality: false }, { isMunicipality: null }],
+        };
+
+    return await this.recyclingPartnerModel.findAll({
+      where: whereCondition,
+    });
+  } catch (error) {
+    throw new Error(`Failed to fetch recycling partners: ${error.message}`);
+  }
 }

 @Query(() => [RecyclingPartnerModel], {
   name: 'skilavottordAllRecyclingPartners',
 })
-async getAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
+async skilavottordAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
   return this.recyclingPartnerService.findAll()
 }

-      if (isMunicipalityPage) {
-        router.push(routes.municipalities.baseRoute).then(() => {
-          toast.success(t.recyclingCompany.add.added)
-        })
-      } else {
-        router.push(routes.recyclingCompanies.baseRoute).then(() => {
-          toast.success(t.recyclingCompany.add.added)
-        })
-      }
+      const route = isMunicipalityPage
+        ? routes.municipalities.baseRoute
+        : routes.recyclingCompanies.baseRoute
+      router.push(route).then(() => {
+        toast.success(t.recyclingCompany.add.added)
+      })

 const {
   control,
   handleSubmit,
   formState: { errors },
 } = useForm({
   mode: 'onChange',
-  defaultValues: {
-    isMunicipality: isMunicipalityPage,
-    municipalityId: partnerId,
-  },
+  defaultValues: isMunicipalityPage
+    ? { isMunicipality: true }
+    : { isMunicipality: false, municipalityId: partnerId || '' },
 })

 const roles = Object.keys(AccessControlRole)
-  .filter((role) =>
-    !hasDeveloperRole(user?.role) ? role !== Role.developer : role,
-  )
-  .filter((role) => {
-    if (hasMunicipalityRole(Role.municipality)) {
-      return (
-        role === Role.recyclingCompany ||
-        role === Role.recyclingCompanyAdmin ||
-        role === Role.municipality
-      )
-    }
-    return role
-  })
+  .filter((role) => {
+    if (!hasDeveloperRole(user?.role) && role === Role.developer) {
+      return false
+    }
+    if (hasMunicipalityRole(user?.role)) {
+      return [
+        Role.recyclingCompany,
+        Role.recyclingCompanyAdmin,
+        Role.municipality,
+      ].includes(role as Role)
+    }
+    return true
+  })
   .map((role) => ({
     label: getRoleTranslation(role as Role, activeLocale),
     value: role,
   }))

 const error =
-  recyclingPartnerError || accessControlsError || recyclingPartnerByIdError
 const loading =
-  recyclingPartnerLoading || accessControlsLoading || recyclingPartnerByIdLoading
 const isData = 
-  !!recyclingPartnerData || !!accessControlsData
+  recyclingPartnerError || accessControlsError || recyclingPartnerByIdError
+  recyclingPartnerLoading || accessControlsLoading || recyclingPartnerByIdLoading
+  !!recyclingPartnerData || !!recyclingPartnerByIdData || !!accessControlsData

#!/bin/bash
# Description: Search for any usages of 'deregisterVehicleKM' in the codebase.

# Test: Find all occurrences of 'deregisterVehicleKM'
rg 'deregisterVehicleKM'

#!/bin/bash
# Let's try a case-insensitive search and also look for kebab-case variations
rg -i "deregister.*vehicle.*km|deregister-vehicle-km"

# Also search for any files with similar naming patterns
fd -i "deregister.*vehicle.*km"

#!/bin/bash
# Check if municipality IDs match company IDs in the codebase
rg -A 5 "companyId.*municipality" 

#!/bin/bash
# Let's check the schema/types and usages to understand the relationship
# First, let's find type definitions related to municipalities and recycling partners
ast-grep --pattern 'interface $_Municipality {
  $$$
}'

# Also check for any type with companyId
ast-grep --pattern 'interface $_ {
  companyId: $_
  $$$
}'

# And check how municipalities are used with recycling partners
rg -B 3 -A 3 "municipalities.*recyclingPartner" 

#!/bin/bash
# Let's check the component's props and the municipalities type
ast-grep --pattern 'type $_Props = {
  $$$
  municipalities: $_
  $$$
}'

# Check how municipalities are structured
rg -B 2 -A 2 "municipalities.*value.*label"

# Look for recyclingPartner type definition
ast-grep --pattern 'type $_RecyclingPartner = {
  $$$
}'

#!/bin/bash
# Let's check the interface definitions and type declarations
ast-grep --pattern 'interface $_ {
  municipalities: $_
  $$$
}'

# Look for municipality option type definitions
rg -B 3 -A 3 "type.*Municipality"

# Check how the municipalities array is populated
rg -B 3 -A 3 "municipalities.*=" 

#!/bin/bash
# Check NavigationLinks usage across the codebase
rg -A 5 "NavigationLinks.*activeSection"

#!/bin/bash
# Check NavigationLinks implementation
ast-grep --pattern 'const NavigationLinks = $_'

# Check for any auth/role checks in the component
rg -A 5 "auth|role|permission" apps/skilavottord/web/components/NavigationLinks

#!/bin/bash
# Check role utility implementation
ast-grep --pattern 'export const hasMunicipalityRole = $_'

# Check UserContext implementation
ast-grep --pattern 'export const UserContext = $_'

# Check if there are any other role-based checks
rg "user?.role" apps/skilavottord/web

#!/bin/bash
# Check PageHeader implementation for accessibility attributes
ast-grep --pattern 'export const PageHeader = $_ => {
  $$$
  return (
    $$$
    <h1 $_>$$$</h1>
    $$$
  )
}'

#!/bin/bash
# Let's try a broader search for PageHeader component
fd PageHeader --type f --exec cat {}

# Also search for any h1 tags in the codebase to understand heading hierarchy
ast-grep --pattern '<h1 $_>$$$</h1>'

# Search for PageHeader usage patterns
rg "PageHeader" -A 2

#!/bin/bash
# Description: Check for other instances of partnerId assignment for municipalities
rg "partnerId.*municipality" -A 3

#!/bin/bash
# Check implementation of hasMunicipalityRole and related role checks
ast-grep --pattern 'function hasMunicipalityRole($_) { $$$ }'

# Check Role enum definition
rg "enum.*Role" -A 10

# Check other usages of user?.role with municipality
rg "user\?\.role.*municipality" -A 2

#!/bin/bash
# Search for Role enum in skilavottord
rg "enum.*Role" -A 5 apps/skilavottord/

# Search for hasMunicipalityRole implementation
ast-grep --pattern 'const hasMunicipalityRole = $_' apps/skilavottord/web/

# Check other role-related functions in the same context
rg "Role\." -A 2 apps/skilavottord/web/screens/RecyclingCompanies/

#!/bin/bash
# Get complete Role enum definition
rg -A 10 "export enum Role" apps/skilavottord/

# Check all usages of municipality role
rg "Role\.municipality" apps/skilavottord/

# Check implementation of hasMunicipalityRole and its imports
rg -A 3 "hasMunicipalityRole" apps/skilavottord/

  "municipalities": {
    "title": "Municipalities",
+   "info": "Information about registered municipalities.",
+   "tableHeaders": {
+     "name": "Name",
+     "address": "Address",
+     "postnumber": "Postal Code",
+     "email": "Email",
+     "status": "Status"
+   },
+   "status": {
+     "active": "Active",
+     "inactive": "Inactive"
+   },
+   "buttons": {
+     "add": "Add Municipality",
+     "edit": "Edit",
+     "delete": "Delete"
+   },
    "municipality": {
      "view": {
        "title": "Municipality",
        "breadcrumb": "View",
        "updated": "Updated",
        "deleted": "Deleted"
      },
      "add": {
        "title": "Add Municipality",
        "breadcrumb": "Add",
        "added": "Added"
      }
    }
  }

-        "info": "Þú getur annaðhvort uppfært eða eytt móttökuaðila. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+        "info": "You can either update or delete a municipality. Please verify that the municipality information is correct.",
-        "info": "Þegar nýju sveitarfélagi er bætt við, verður hann aðgengilegur í aðgangsstýringu. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+        "info": "When a new municipality is added, it will be available in access control. Please verify that the municipality information is correct.",

  "municipality": {
    "label": "Sveitarfélag",
-   "placeholder": "Sveitarfélag"
+   "placeholder": "Sveitarfélag",
+   "rules": {
+     "required": "Skylda er að fylla út sveitarfélag"
+   }
  }

  "municipalities": {
    "title": "Sveitarfélög",
+   "info": "Upplýsingar um skráð sveitarfélög.",
    "empty": "Ekkert sveitarfélag fannst",
+   "tableHeaders": {
+     "name": "Nafn",
+     "address": "Heimilisfang",
+     "postnumber": "Póstnúmer",
+     "email": "Netfang",
+     "status": "Staða"
+   },
+   "status": {
+     "active": "Virkt",
+     "inactive": "Óvirkt"
+   },
+   "buttons": {
+     "add": "Bæta við sveitarfélagi",
+     "edit": "Breyta",
+     "delete": "Eyða"
+   },
    "municipality": {
      "view": {
        "title": "Sveitarfélag",
        "breadcrumb": "Skoða",
-       "info": "Þú getur annaðhvort uppfært eða eytt móttökuaðila. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+       "info": "Þú getur annaðhvort uppfært eða eytt sveitarfélagi. Vinsamlegast staðfestið að upplýsingar sveitarfélags séu rétt slegnar inn.",
        "updated": "Uppfært",
        "deleted": "Eytt"
      },
      "add": {
        "title": "Bæta við sveitarfélagi",
        "breadcrumb": "Bæta við",
        "info": "Þegar nýju sveitarfélagi er bætt við, verður það aðgengilegt í aðgangsstýringu. Vinsamlegast staðfestið að upplýsingar sveitarfélags séu rétt slegnar inn.",
        "added": "Bætt við"
-     }
+     },
+     "form": {
+       "inputs": {
+         "name": {
+           "label": "Nafn",
+           "placeholder": "Nafn sveitarfélags",
+           "rules": {
+             "required": "Skylda er að fylla út nafn"
+           }
+         },
+         "email": {
+           "label": "Netfang",
+           "placeholder": "Netfang",
+           "rules": {
+             "required": "Skylda er að fylla út netfang",
+             "validate": "Netfang er ekki á réttu formi"
+           }
+         }
+       }
+     }
    }
  }

apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (3)

55-66: Add error handling for the data transformation

The municipality data transformation could be more robust by handling potential undefined values and empty arrays.

Consider updating the transformation:

- const municipalities = data?.skilavottordRecyclingPartners.map(
+ const municipalities = data?.skilavottordRecyclingPartners?.map(
    (municipality) => ({
      label: municipality.companyName,
      value: municipality.companyId,
    }),
-  )
+  ) ?? []

---

281-315: Enhance accessibility for the municipality select component

While the implementation is functionally correct, consider adding ARIA attributes for better accessibility.

Add these attributes to the Select component:

 <Select
   name={name}
   label={t.recyclingCompany.form.inputs.municipality.label}
   placeholder={t.recyclingCompany.form.inputs.municipality.placeholder}
+  aria-label={t.recyclingCompany.form.inputs.municipality.label}
+  aria-required="true"
   size="md"
   value={municipalities?.find((option) => option.value === value)}
   hasError={!!errors?.municipality?.message}
   errorMessage={errors?.municipality?.message}
   backgroundColor="blue"
   options={municipalities}
   onChange={onChange}
   isCreatable
   isDisabled={hasMunicipalityRole(user?.role)}
 />

---

340-365: Consider using styled components instead of inline styles

While the implementation correctly maintains the checkbox in the DOM as required, the visibility control could be improved.

Consider using styled components or CSS classes:

- <div
-   style={{
-     display: user?.role && hasDeveloperRole(user.role) ? 'block' : 'none',
-   }}
- >
+ <Box
+   display={user?.role && hasDeveloperRole(user.role) ? 'block' : 'none'}
+ >

apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)

81-84: Consider preserving form state on modal close

The current implementation clears the form whenever the modal is reopened. Consider if this is the desired behavior or if form state should be preserved in certain scenarios.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.

 @Query(() => [RecyclingPartnerModel], {
   name: 'skilavottordAllRecyclingPartners',
 })
-async getAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
+async skilavottordAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
   return this.recyclingPartnerService.findAll()
 }

 async getRecyclingPartners(
   @Args('isMunicipalityPage', { type: () => Boolean, nullable: true })
   isMunicipalityPage: boolean,
   @Args('municipalityId', { type: () => String, nullable: true })
   municipalityId: string | null,
 ): Promise<RecyclingPartnerModel[]> {
+  if (municipalityId && !municipalityId.trim()) {
+    throw new BadRequestException('municipalityId cannot be empty')
+  }
+  try {
     return this.recyclingPartnerService.findRecyclingPartners(
       isMunicipalityPage,
       municipalityId,
     )
+  } catch (error) {
+    throw new InternalServerErrorException(
+      `Failed to fetch recycling partners: ${error.message}`,
+    )
+  }
 }

#!/bin/bash
# Description: Verify consistent role implementation and guard usage

# Check for Role.municipality usage
rg -A 2 "Role\.municipality" 

# Check for proper guard implementation
ast-grep --pattern 'class $_ implements CanActivate {
  $$$
  Role.municipality
  $$$
}'

#!/bin/bash
# Check for the authorization guard implementation
ast-grep --pattern 'class AuthorizeGuard implements CanActivate {
  $$$
}'

# Check for role enum definition
rg -A 5 "export enum Role"

# Check for any custom municipality-specific guards
rg -l "municipality.*guard"

#!/bin/bash
# Check for the authorization guard implementation
rg -A 10 "class.*Authorize.*Guard" 

# Check for municipality-specific authorization logic
rg -A 5 "canActivate.*municipality"

# Check for the Authorize decorator implementation
ast-grep --pattern '@Authorize({
  $$$
})'

#!/bin/bash
# Check if municipality and recycling partner IDs match the assumed pattern
ast-grep --pattern 'companyId: string' # Search for company ID type definitions
rg -A 2 'municipality.*companyId|recyclingPartner.*companyId' # Search for ID usage patterns

#!/bin/bash
# Check municipality and recycling partner type definitions and data transformations
ast-grep --pattern 'interface Municipality {
  $$$
}'

ast-grep --pattern 'interface RecyclingPartner {
  $$$
}'

# Check how municipalities are mapped in the codebase
rg -A 3 'municipalities.*map' 

#!/bin/bash
# Check recycling partner data structure and municipality relationships
ast-grep --pattern 'type RecyclingPartnerDTO = {
  $$$
}'

# Look for municipality-recycling partner relationship definitions
rg -A 3 'skilavottordRecyclingPartners'

 const getPartnerId = (
-  municipalityId: string,
-  partnerId: string,
+  municipalityId: string | null | undefined,
+  partnerId: string | null | undefined,
   role: Role,
 ) => {
+  if (!user) {
+    throw new Error('User context is required')
+  }
+
   // If the user has municipality role, then he can only create a new access under the same municipality
   if (hasMunicipalityRole(user?.role) && hasMunicipalityRole(role)) {
-    return user.partnerId
+    return user.partnerId ?? null
   }

   // If selected role is municipality, use municipalityId, else use partnerId
-  return hasMunicipalityRole(role) ? municipalityId : partnerId || null
+  return hasMunicipalityRole(role) ? municipalityId ?? null : partnerId ?? null
 }

apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)

97-100: ⚠️ Potential issue

Add null check for role value

The role value check should handle undefined cases.

       partnerIdRequired={
-        watch('role')?.value === Role.recyclingCompanyAdmin ||
-        watch('role')?.value === Role.recyclingCompany
+        (watch('role')?.value === Role.recyclingCompanyAdmin ||
+         watch('role')?.value === Role.recyclingCompany) ?? false
       }

apps/skilavottord/web/utils/accessUtils.ts (3)

4-9: Add JSDoc documentation for better type information and usage guidance.

The function signature is well-typed, but adding JSDoc would improve developer experience by documenting parameter requirements and return values.

+/**
+ * Determines the appropriate partner ID based on user role and provided parameters
+ * @param user - The current user object
+ * @param municipalityId - The ID of the municipality if applicable
+ * @param partnerId - The partner ID if applicable
+ * @param role - The role being evaluated
+ * @returns The determined partner ID or null
+ */
 export const getPartnerId = (
   user: SkilavottordUser | undefined,
   municipalityId: string | null,
   partnerId: string | null,
   role: Role,
 ) => {

---

10-13: Consider making the role check more explicit.

The conditional logic is correct, but the comment could be more precise about the business rule being enforced.

-  // If the user has municipality role, then he can only create a new access under the same municipality
+  // Municipality users can only create access within their assigned municipality
   if (hasMunicipalityRole(user?.role) && hasMunicipalityRole(role)) {
     return user.partnerId
   }

---

15-17: Consider making null coalescing more explicit with early returns.

The ternary with null coalescing could be more readable with explicit conditions.

-  return hasMunicipalityRole(role) ? municipalityId ?? null : partnerId ?? null
+  if (hasMunicipalityRole(role)) {
+    return municipalityId ?? null
+  }
+  return partnerId ?? null

apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)

85-89: Simplify the partnerIdRequired logic

The current implementation can be made more concise and readable.

-      partnerIdRequired={
-        (watch('role')?.value === Role.recyclingCompanyAdmin ||
-          watch('role')?.value === Role.recyclingCompany) ??
-        false
-      }
+      partnerIdRequired={[Role.recyclingCompanyAdmin, Role.recyclingCompany].includes(watch('role')?.value ?? '')}

apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)

81-96: Enhance JSDoc documentation

While the implementation is good, the JSDoc comments could be more descriptive.

   /**
    * Get the id of the municipality or recyclingpartner that is paying for the recycling request
-   * @param companyId
-   * @returns
+   * @param {string} companyId - The unique identifier of the company
+   * @returns {Promise<string>} The ID of the paying partner (municipality or recycling partner)
+   * @throws {Error} When no partner is found for the given company ID
    */

apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)

61-64: Simplify municipality ID lookup

The find logic can be made more concise.

-      municipalityId: municipalities.find(
-        (option) =>
-          option.value === currentPartner?.recyclingPartner?.companyId,
-      ),
+      municipalityId: municipalities.find(option => option.value === currentPartner?.recyclingPartner?.companyId),

apps/skilavottord/web/i18n/locales/en.json (2)

395-396: Consider more descriptive button labels.

The "view" label could be more descriptive. Consider using "View details" or "View company" to provide more context to users.

---

647-662: Enhance descriptive text for municipality management.

Consider adding more context to the following strings:

• Add a description of what municipalities are used for in the system
• Clarify the impact of adding/updating municipalities

apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)

37-56: Add parameter documentation and validation

While the implementation looks good, consider these improvements:

1. Add JSDoc comments to document the parameters and their relationship
2. Add validation for the case when municipalityId is provided

 @Query(() => [RecyclingPartnerModel], {
   name: 'skilavottordRecyclingPartners',
 })
+/**
+ * Retrieves recycling partners based on municipality page status and ID
+ * @param isMunicipalityPage - Whether to fetch municipality-specific partners
+ * @param municipalityId - Optional municipality ID, can be null for recyclingFund role
+ */
 async skilavottordRecyclingPartners(
   @Args('isMunicipalityPage', { type: () => Boolean, nullable: true })
   isMunicipalityPage: boolean,
   @Args('municipalityId', { type: () => String, nullable: true })
   municipalityId: string | null,
 ): Promise<RecyclingPartnerModel[]> {
   try {
+    if (municipalityId && !isMunicipalityPage) {
+      throw new BadRequestException(
+        'municipalityId can only be provided when isMunicipalityPage is true',
+      )
+    }
     return this.recyclingPartnerService.findRecyclingPartners(
       isMunicipalityPage,
       municipalityId,
     )

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)

15-19: Consider grouping related auth imports

The auth-related imports (hasPermission, hasMunicipalityRole) could be combined into a single import statement for better organization.

-import {
-  hasPermission,
-  hasMunicipalityRole,
-} from '@island.is/skilavottord-web/auth/utils'
+import { hasPermission, hasMunicipalityRole } from '@island.is/skilavottord-web/auth/utils'

---

161-161: Consider alternative to delete operator

Using the delete operator can impact performance. Consider using undefined assignment instead.

-delete input.__typename
+input.__typename = undefined

🧰 Tools

🪛 Biome (1.9.4)

[error] 161-161: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (3)

26-27: Add explicit type annotation for better type safety

The partnerIds variable should have an explicit type annotation to prevent potential type-related issues.

-    let partnerIds = []
+    let partnerIds: string[] | null = []

---

28-48: Improve comment clarity for municipality partner handling

The implementation looks good with proper error handling and query optimization. However, the comments could be more descriptive.

-    // Get all sub recycling partners of the municipality
-    // else get all
+    // If partnerId is provided, fetch all sub-recycling partners associated with the municipality
+    // Otherwise, return null to fetch all partners

---

61-64: Fix indentation for better readability

The object spread operator's indentation is inconsistent with the surrounding code.

             ...(partnerIds
               ? {
-                  recyclingPartnerId: partnerIds,
+                recyclingPartnerId: partnerIds,
                }
               : {}),

apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)

122-130: Refactor duplicated success handling

The success handling contains duplicated toast message code.

Apply this diff to reduce code duplication:

if (!errors) {
-  if (isMunicipalityPage) {
-    router.push(routes.municipalities.baseRoute).then(() => {
-      toast.success(t.recyclingCompany.add.added)
-    })
-  } else {
-    router.push(routes.recyclingCompanies.baseRoute).then(() => {
-      toast.success(t.recyclingCompany.add.added)
-    })
-  }
+  const route = isMunicipalityPage
+    ? routes.municipalities.baseRoute
+    : routes.recyclingCompanies.baseRoute
+  router.push(route).then(() => {
+    toast.success(t.recyclingCompany.add.added)
+  })
}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts:32-45
Timestamp: 2024-12-16T12:25:52.236Z
Learning: In `apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts`, the `municipalityId` parameter in `getRecyclingPartners` can be `null` for users with the `recyclingFund` role.

Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/i18n/locales/is.json:0-0
Timestamp: 2024-12-16T12:38:27.802Z
Learning: In the Skilavottord web application, the form for recycling partners is reused for municipalities. Therefore, specific translations for municipalities' form inputs are not needed in `apps/skilavottord/web/i18n/locales/is.json`.

#!/bin/bash
# Description: Check for consistency in municipality translations across the application

# Search for municipality-related translations in locale files
echo "Checking locale files for municipality translations..."
rg -i "municipality|sveitarfélag" apps/skilavottord/web/i18n/locales/

# Search for municipality role usage in components
echo "Checking components for municipality role usage..."
rg "Role\.municipality" apps/skilavottord/web/